-
Notifications
You must be signed in to change notification settings - Fork 177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable Trimmable Assembly #391
Conversation
@josephdecock can you take a look at this one too? |
@josephdecock could you take a look at the latest iteration? Thanks! |
Ping |
I was still interested in why we need multi-targeting, so I pulled this branch down and tried compiling without the net6.0 TFM. I got this warning:
So, should we add that condition? I also don't see why we don't just say |
This is needed to make this assembly trimmable
For future reference: this seems to be unnecessary, because we aren't targeting more specific platforms. If we needed to target something like net6.0-windows, then these more complex conditions would make sense, but we don't, so we can keep it simple. |
public static string Serialize<T>(T logObject) | ||
{ | ||
return Enabled ? JsonSerializer.Serialize(logObject, JsonOptions) : "Logging has been disabled"; | ||
} | ||
return Enabled ? | ||
JsonSerializer.Serialize(logObject, (JsonTypeInfo<T>)SourceGenerationContext.Default.GetTypeInfo(typeof(T))) : | ||
"Logging has been disabled"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change because the LogSerializer can now only serialize the types in the source generator context. That works fine for our internal usage of the LogSerializer, but the LogSerializer is a public type. Anyone who is using this helper to serialize anything else will get an NRE when GetTypeInfo returns null.
Options:
- Make the LogSerializer internal.
- Don't enable trimmable
- Make this method private, but keep the original
Serialize(object logObject)
as a public overload with[RequiresUnreferencedCode]
Changing the log serializer to support trimming means that we need to add the types that we will serialize into a serialization context. Unfortunately, the serializer is a public type that serializes arbitrary objects today. This is incompatible with trimming. The solution is to mark the existing API as [RequiresExternalCode], which gives trimmed callers of the API a warning, and to add overloads that accept the types that are in the generation context.
With this change I am annotating the core assembly as IsTrimmable. This enables warnings for any known issues that may arise when publishing a final project as trimmed. The only warnings present were the use of runtime reflection code to perform the Json serialization. I converted over to using the Json Source Generator to move this logic from runtime to compile time (a net performance win too).
fixes #390